-
Notifications
You must be signed in to change notification settings - Fork 891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] share ayah as audio #2011
Conversation
* attempt 1 * attempt 2 * attempt 3 * clean up 1 * attempt 3 * attempt 4 * cleanup * cleanup 2
|
Notes on the icons:
Other notes:
جزاكم الله خيرا |
shareIntent.putExtra(Intent.EXTRA_STREAM, uri) | ||
shareIntent.type = "audio/mp3" | ||
shareIntent.addFlags(Intent.FLAG_GRANT_READ_URI_PERMISSION) | ||
activity.startActivity(Intent.createChooser(shareIntent, "Share")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "Share", You could use one of the following (and don't forget to make it localizable)
- Share Audio File
- Share Recitation of Husary
- Share Recitation of Husary for 1:1 - 1:5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Share Audio File
Done
Thanks you for the advise @benomaire , I shall act accordingly |
|
I have renamed the audio file name to the pattern [qari_name]_[startsurah]-[startayah]_[endsurah]-[endayah].mp3 eg. minshawi_murattal_113-5_114-2.mp3 |
|
Fixed bullet 3 |
|
|
Issue 4 fixed (I guess) |
|
السلام عليكم ورحمة الله وبركاته with respect to how we're splitting the mp3 from the mp3, did you try the |
وَعَلَيْكُمُ ٱلسَّلَامُ |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jazakumAllah khairan - did a first pass review with some initial comments.
regarding the built in media library lag, were there any complaints online or any bug tracker issues? if so, anything we can leverage to fix it? if not, are we sure we are using it properly?
only reason I ask is because if we don't have to maintain CheapMp3 classes, would be better.
@@ -102,9 +102,11 @@ | |||
android:authorities="@string/file_authority" | |||
android:grantUriPermissions="true" | |||
android:exported="false"> | |||
tools:replace="android:authorities"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we need to add this?
<meta-data | ||
android:name="android.support.FILE_PROVIDER_PATHS" | ||
android:resource="@xml/file_paths"/> | ||
android:resource="@xml/file_paths" | ||
tools:replace="android:resource" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this
import java.io.* | ||
import java.util.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please let's not use star imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed this in the latest commit
val destFile = File(PagerActivity.audioCacheDirectory.path + File.separator + tempAudioName) | ||
try { | ||
val fileInputStream = FileInputStream(path1) | ||
val bArr = ByteArray(1048576) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constant - also what's relevant about this number here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 1 MB (1024*1024 bytes), but I have removed this in favor of "Okio"
val fileOutputStream = FileOutputStream(destFile) | ||
while (true) { | ||
val read = fileInputStream.read(bArr) | ||
if (read == -1) { | ||
break | ||
} | ||
fileOutputStream.write(bArr, 0, read) | ||
fileOutputStream.flush() | ||
} | ||
fileInputStream.close() | ||
val fileInputStream2 = FileInputStream(path2) | ||
while (true) { | ||
val read2 = fileInputStream2.read(bArr) | ||
if (read2 == -1) { | ||
break | ||
} | ||
fileOutputStream.write(bArr, 0, read2) | ||
fileOutputStream.flush() | ||
} | ||
fileInputStream2.close() | ||
fileOutputStream.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have okio in the code that might simplify this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
kotlin.Pair pair2 = getReorderedAyatPair(start, end); | ||
selectedStartSuraAyah = (SuraAyah) pair2.component1(); | ||
selectedEndSuraAyah = (SuraAyah) pair2.component2(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val (startSuraAyah, endSuraAyah) = getReorderedAyatPair(start, end)
return pair; | ||
} | ||
|
||
private boolean audioFilesExist(AudioPathInfo audioPathInfo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure we have something already doing this somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, in AudioUtils
. I felt putting the code in a method will help on readability about whats going on
|
||
private void createAndShareAudio(SuraAyah start, SuraAyah end, AudioPathInfo audioPathInfo) { | ||
showProgressDialog(); | ||
compositeDisposable.add( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's prefer coroutines to Rx - you'll be able to do that easier when we move out of this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the new AudioShareUtils
class in the feature:audio
@Override | ||
public void onSuccess( | ||
@io.reactivex.rxjava3.annotations.NonNull ArrayList<SparseIntArray> sparseIntArrayList) { | ||
Intrinsics.checkNotNullExpressionValue(sparseIntArrayList, "mapArray"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can't be null - if it were null it'd crash, rx2+ doesn't support null emissions
); | ||
} | ||
|
||
private ArrayList<SparseIntArray> getTimingData(SuraAyah start, SuraAyah end, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type of thing should probably live in another module - common/audio (already exists) that we can add a timing package there and have these methods - later, the service could be updated to use this instead of replicating its code
|
|
hello akhi @ahmedre |
JazakumAllah khairan will try to review this weekend in sha' Allah |
given the most recent changes to the main branch, some changes need to be made before this can be merged. these changes are in progress and will in sha' Allah update this branch with the changes and try to get this merged in the near future in sha' Allah. |
let's continue the discussion in #2072 so I can push updates. |
This is an attempt to address issue #1731
Screenshot
Addressed
Not Addressed